Fix: fix loadimage function to robustly load image #34

stevenhua0320 · 2025-10-25T02:40:32Z

Closes #4 @sbillinge Ready to review

codecov · 2025-10-25T02:43:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f454574) to head (c8b83e5).
⚠️ Report is 40 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##             main       #34       +/-   ##
============================================
+ Coverage   50.00%   100.00%   +50.00%     
============================================
  Files           2         3        +1     
  Lines          18        48       +30     
============================================
+ Hits            9        48       +39     
+ Misses          9         0        -9

Files with missing lines	Coverage Δ
tests/conftest.py	`100.00% <100.00%> (+64.28%)`	⬆️
tests/test_load_image.py	`100.00% <100.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbillinge

@stevenhua0320 I can't see the tests for this. I guess that you started by writing a test, which is our workflow, right? :)

you can work with @zmx27 to get help with this, but

there is a tif file in docs/examples/data (actually, that is where it should be)
in conftest.py, copy this into the user_filesystem soemwhere
use this fixture in test to try the different scenarios (directory is cwd, directory is absolute path, directory is relative path0

stevenhua0320 · 2025-10-25T17:11:27Z

@stevenhua0320 I can't see the tests for this. I guess that you started by writing a test, which is our workflow, right? :)

you can work with @zmx27 to get help with this, but

there is a tif file in docs/examples/data (actually, that is where it should be)

in conftest.py, copy this into the user_filesystem soemwhere

use this fixture in test to try the different scenarios (directory is cwd, directory is absolute path, directory is relative path0

Actually I have tested with these scenarios on my local machine and they generated the correct matrix of that same tif file. But, you are right. It is more rigorous to write a test here.

sbillinge · 2025-10-25T17:16:30Z

Actually I have tested with these scenarios on my local machine and they generated the correct matrix of that same tif file. But, you are right. It is more rigorous to write a test here.

yes, this is our workflow. Agree on the test. Only then write the function. The test captures the behavior we want (so in this case, three different successful cases). We can also think about how to handle unsuccessful cases. Probably give a helpful error message for FileNotFound or whatever. The error messages all have the form "what happened. What to do to fix it", so here it would be something like f"The file {file_path} cannot be found. Please rerun specifying a valid filename". There may also be an error reading the file, which would fail in a different way and require a different error message. We put "failures" in a different test.

stevenhua0320 · 2025-10-25T17:53:00Z

@sbillinge I'm designing for the test, right now it works well

Please rerun specifying a valid filename

Got the idea, in the original function, it has the FileNotFoundError and gives the error message. I'm still working on the good cases and also I have some ideas that how we gonna test the bad case. We probably can specify a file that is nowhere in the system (ex:nonexistent_file.tif) and let the function to find it. The desired matrix it should give is a matrix full of 0 as described in the original function. I'm still working on those test cases.

sbillinge · 2025-10-25T18:22:31Z

that's the right idea.

We would usually build the desired user-filesystem as a fixture in conftest.py. In this way it can be reused across different tests. Take a look in diffpy.labpdfproc or diffpy.cmi to see examples.

sbillinge

Se comments

sbillinge · 2025-10-25T21:22:59Z

tests/conftest.py

-    cwd_dir = base_dir / "cwd_dir"
-    cwd_dir.mkdir(parents=True, exist_ok=True)
+    test_dir = base_dir / "test_dir"
+    for d in (input_dir, home_dir, test_dir):


Don't use a single letter for the iterator. Always choose to make it more readable... E.g., for dir in (

I am not sure if you used an LLM for help. OK if you did, but try and work hard to bring the code up to group standards before pushing. It is really really tedious reviewing code from chatgpt....

Sorry, that is my habit of indexing object when writing for loops. I should stop this formatting and follow more readable format.

sbillinge · 2025-10-25T21:24:19Z

tests/conftest.py

+def example_tif():
+    """Locate the example TIFF file relative to repo root."""
+    tif_path = Path("../docs/examples/KFe2As2-00838.tif").resolve()
+    if not tif_path.exists():


Ooooh, don't be doing that. We want tests to fail of something is wrong

I will delete this in the next PR.

sbillinge · 2025-10-25T21:24:49Z

tests/conftest.py

+    return LoadImage(cfg)
+
+
+# ----------------------------------------------------------------------


Please remove all these comment blocks

sbillinge · 2025-10-25T21:38:57Z

tests/conftest.py

+# LoadImage test setup
+# ----------------------------------------------------------------------
+@pytest.fixture(scope="module")
+def example_tif():


we don't need a fixture for htis. Just copy the file over as you build the filesystem above.

sbillinge · 2025-10-25T21:40:46Z

tests/conftest.py

+    return tif_path
+
+
+@pytest.fixture(scope="module")


i am not sure what this is doing but it doesn't seem appropriate to be building a fixture using one of hte functions that we will be testing. Just delete this.

As a heads-up, we don't do any testing in conftest.py we just build fixtures.

So, you mean we need to create a new test_loadimage.py and do all the tests there? If yes then I migrate the test to there.

sbillinge · 2025-10-25T21:42:06Z

tests/conftest.py

+    return loader.loadImage(example_tif)
+
+
+@pytest.fixture


see above, this should also be deleted.

sbillinge · 2025-10-25T21:43:07Z

tests/conftest.py

+# Unified test
+# ----------------------------------------------------------------------
+
+


use pytest.mark.parametrize for the different cases. Please see examples in diffpy.cmi etc.

sbillinge · 2025-10-25T21:44:01Z

tests/conftest.py

+):
+    home_dir = user_filesystem["home"]
+    test_dir = user_filesystem["test"]
+    monkeypatch.setenv("HOME", str(home_dir))


we don't use monkeypatch but we use pytest mock if it is needed.

sbillinge

please see comments.

stevenhua0320 · 2025-10-26T01:04:42Z

please see comments.

I have seen it and I'm currently making edits so that it would follow group's standard.
@sbillinge Really sorry professor I found I'm not so familiar with the use of how Pathlib actually works. It takes me a long time to think about the structure of the tree of files. Maybe still I need to refine the test if it is necessary. For the reason I can only make tags in the pytest.mark.parametrize is because in the case of absolute path, it must first go to its parent to mock to create the folder. That's why it cannot cluster the case of relative path and non-existance path together to make the same workflow of testing as far as I learned.

sbillinge

pls see comments

sbillinge · 2025-10-26T02:12:43Z

tests/test_loadimage.py

+    return image_loader.loadImage(tif_path)
+
+
+def build_loadimage_path(case_tag, home_dir):


your test seems to have logic in it, but we don't really want that. the purpose of the tests is to encode behavior and then test logic in the function. The tests just have to capture behavior. please see below.

sbillinge · 2025-10-26T02:25:46Z

tests/test_loadimage.py

+
+
+load_image_param = load_image_params = [
+    ("abs", PROJECT_ROOT / "docs/examples/KFe2As2-00838.tif"),


this is where we capture the behavior. Please see the other projects for examples, but we want first a comment, e.g.

# case 1: just filename of file in current directory. expect function loads tiff file from cwd

then the paramaterize looks something like (input, output). In this case the input would be "KFe2As2-00838.tif" and the output would assert something that assures us that the file was correctly read.

Then similarly for case 2 # case 2: absolute file path to file in another directory. expect file is found and correctly read
and case 3 would be the relative path.

the test would look soething like

@pytest.mark.parametrize("input, expected") def test_load_image_cases(input, expected, , user_filesystem): copy the test tiff to home_dir copy the test tiff to test_dir # these two lines could be done in conftest.py if we want cd to home_dir actual = load_image(input) some asserts that compare actual and expected things.

stevenhua0320 · 2025-10-26T03:36:52Z

@sbillinge Professor Simon, If there is anything I could modify in the test file please give me more feedback and I would continue to change it.

sbillinge

the cases captured in the comments are now good, except Case 4 which I think should result in a crash, but the code needs some tlc. Please could you reach out to @zmx27 and work together on the implementation?

stevenhua0320 · 2025-10-26T16:21:26Z

the cases captured in the comments are now good, except Case 4 which I think should result in a crash, but the code needs some tlc. Please could you reach out to @zmx27 and work together on the implementation?

OK，actually in Case 4 in the implementation it would return the message like "file {filename} not found, please type in correct file name..." something like that and then return nothing. That's why in the test cases I check the datatype of what loadimage would return. I would reach out to Zhiming @zmx27 to see whether the implementation of loadimage needs further update.

sbillinge · 2025-10-26T16:48:30Z

the cases captured in the comments are now good, except Case 4 which I think should result in a crash, but the code needs some tlc. Please could you reach out to @zmx27 and work together on the implementation?

OK，actually in Case 4 in the implementation it would return the message like "file {filename} not found, please type in correct file name..." something like that and then return nothing. That's why in the test cases I check the datatype of what loadimage would return. I would reach out to Zhiming @zmx27 to see whether the implementation of loadimage needs further update.

yes, I know that was your intent with case 4. I would rather that the users get that message but in a crash. In that case it would be in the test_function_bad. The next step is to make a clean PR from main with just the tests in it and no edits to loadimage. Then we can work on loadimage, but it is safer if we don't use any of the edits that were done in the past before the tests. We don't need to check the datatype of what loadimage returns I think, in this PR. EIther it can read a valid file or it can't and will crash.

sbillinge

Good progress. Still some things in the wrong direction.

We do not need ‎tests/cwd/example.tiff, ‎tests/home/example.tiff, or ‎tests/test/example.tiff so please delete these. We will make a clean PR later so we don't add and remove these files, but for now, please delete.

tests/test_load_image.py

sbillinge · 2025-10-27T20:25:08Z

tests/test_loadimage.py

+        )()
+        loader = LoadImage(cfg)
+
+        if expected:


don't have logic in your test

sbillinge · 2025-10-27T20:26:45Z

tests/test_loadimage.py

+@pytest.mark.parametrize("input_path, expected", load_image_param)
+def test_load_image_cases(input_path, expected, user_filesystem):
+    base_dir, home_dir, cwd_dir, test_dir = user_filesystem
+    test_file_dir = Path(__file__).parent


you seem to be working not in tmpdir, so don't be using test_file_dir in the tests. In fact don't even create this directory.

We will create all the directories we need in tmp_dir. Actually, we already have them, they are passed in from user_filesystem.

so we will move to cwd:

os.chdir(cwd_dir)

and we need to move our example file from docs/examples to the different places we will use it. So something like

source_dir = Path(__file__).parent / "docs" / "examples" / "data" shutil.copy(source_dir / "example.tiff", test_dir) shutil.copy(source_dir / "example.tiff", .)

So we have an example tiff in cwd_dir (which is also the current working directory because we cd'd there) and also in ../test_dir.
After this we are fully working in the tempdir that will be built up before every test and removed after every test automatically.

Then we run as

actual_image = load_image(input_path) assert something_about_actual_image == expected

we can decide different things to assert. That the shape is right, the total integrated intensity, and so on.

Professor, you are right. Now I learned how to deal with the conftest for user_system and now it would not create additional files in the step of copying as it is in a tmp_path. I also edited the testing here to both handle good case and bad case without using logic here. I would commit a new PR not introducing additional example.tiff in directory immediately.

sbillinge

Still a bit more to go. We are testing relative paths but not absolute. Also, please see comments

tests/test_load_image.py

sbillinge

please see comments.

sbillinge · 2025-10-30T23:29:55Z

news/load-image-fix.rst

 **Fixed:**

-* <news item>
+* Fixed `loadimage` function in any directory.


load_image() function correctly finds files when passed a relative path

sbillinge · 2025-10-30T23:30:35Z

src/diffpy/srxplanar/loadimage.py

            pic = np.array(pic[::-1, :])
        return pic

    def loadImage(self, filename):


let's make the function have a pep8 compliant name. We may as well fix these when we touch the function.

src/diffpy/srxplanar/loadimage.py

tests/test_load_image.py

sbillinge

pls see comments

tests/test_load_image.py

sbillinge

pls see comments

tests/test_load_image.py

Updated expected values for image loading tests and corrected the source file path.

sbillinge

I edited the file to show what I had in mind. It is breaking the test now for sure but hopefully this gives you the idea.

sbillinge · 2025-10-31T18:51:40Z

btw, I would like to get rid of the cfg clause too. We should decide what we want the function to do explicitly and then test that, so we don't want to put logic in the test that makes the function pass.

stevenhua0320 · 2025-10-31T19:00:13Z

btw, I would like to get rid of the cfg clause too. We should decide what we want the function to do explicitly and then test that, so we don't want to put logic in the test that makes the function pass.

Got the idea, I would integrate the hard-coded one for the first and last point of example tiff file, change it to the data directory, and try to eliminate the cfg clause and make another PR soon.

sbillinge

ok, good progress, we are getting there.

tests/test_load_image.py

sbillinge

pls see comments

sbillinge · 2025-11-01T13:13:24Z

tests/test_load_image.py

 import os
 import shutil
 from pathlib import Path
 from unittest.mock import Mock


we don't ever use unittest.mock, we use pytest.mock, but as discussed below, I don't think we should be mocking anything here.

tests/test_load_image.py

sbillinge

Few more questions on the function now. Can you suggest how we want it to handle flipping?

src/diffpy/srxplanar/loadimage.py

sbillinge · 2025-11-01T17:19:39Z

src/diffpy/srxplanar/loadimage.py

+                    image = np.load(filenamefull)
+                else:
+                    image = open_image(
+                        str(filenamefull)


Per the conversation above, didn't we want to remove this str? That was my point actually

src/diffpy/srxplanar/loadimage.py

sbillinge

I think the flipping regime is passed in when the object is instantiated, so it would be better to keep these things as attributes and not have to specify them

sbillinge · 2025-11-03T13:13:49Z

src/diffpy/srxplanar/loadimage.py

        return

-    def flipImage(self, pic):
+    def flip_image(self, pic, fliphorizontal=False, flipvertical=False):


I think it would be better to not pass the fliphorizontal etc in but just take them from the class attribute that can be set when the class is instantiated

stevenhua0320 · 2025-11-03T14:51:48Z

@sbillinge I think Xiaohao originally used the idea of passing all the attributes to the object, then use flip_image to check whether it needs to be flipped. If both are not, then it gives a default image, and vice versa.

sbillinge

please see my comment, but now I suggest that we don't change the flipping behavior from the current and go back to how it was done before. I was thinking the loader just loaded an image but it obviously also doing a bunch of "magic" on it also. I Don't love how it is doing it but it is not worth the effort to refactor this now I think.

I think you tests should be failing. Do you know why they are passing? The code is, I think not flipping horizontally and vertically now, but yet your tests are still passing. Or is it actually flipping as expected?

sbillinge · 2025-11-03T17:08:50Z

src/diffpy/srxplanar/srxplanar.py

                # rv *= self.correction
        else:
            rv = image
            if flip is True:


this logic needs to be changed in the new structure. Though I see that there is a bunch of logic in here that makes this more complicated than I thought. We should decide if we actually want to do this as it is a larger refactoring than we originally took on.

I think we should take this as a larger refactor because therer are a lot of usages here as I refactor for load_image function to correctly read the directory. And since here in the function flip is defined as a binary variable, most probably here Xiaohao wanted to filter out whether there is fliphorizontal or flipvertical action. If one of them is true then this flip binary variable is true. The problem is that in the load_image function it has integrated the flip_image functionality. So I'm unsure about whether we need to do it again here. For now, I think we should focus on the original process of release and probably create an issue for this.

I think I know what happened here. There were in the old days a program called fit2D and now a program called pyfai and they used different definitions for the origin (the zero) of the image from top left to bottom left or sthg like that, so there was a particular flipping that we would often find. In general, we would like the code to be more general, so a more complete refactor could be done, but I don't think it is worth it as not many people are using this atm. So if we can keep what you have done, because it is good and we have more tests, but make sure that the code kind of behaves the same way as before, so flip can be either True or False and when it is True it is the flip that Xiahao was doing, then if you can assure me that this is the case, I will just merge this.

stevenhua0320 · 2025-11-03T17:28:20Z

please see my comment, but now I suggest that we don't change the flipping behavior from the current and go back to how it was done before. I was thinking the loader just loaded an image but it obviously also doing a bunch of "magic" on it also. I Don't love how it is doing it but it is not worth the effort to refactor this now I think.

I think you tests should be failing. Do you know why they are passing? The code is, I think not flipping horizontally and vertically now, but yet your tests are still passing. Or is it actually flipping as expected?

It should pass the test because now the attributes are assigned to the pictures. When it is in test function, we are given with the flipping behavior of the image, then the load_image fuction would also take the attributes since the flip_image would see whether there is flip horizontal or vertical. Therefore, there is no problem with the current test and there is flips as expected since I changed to what Xiaohao did before to use attributes to pass flip behavior as expected.

sbillinge

please see comment. I think we are very close to being able to merge this.

sbillinge · 2025-11-03T17:39:58Z

src/diffpy/srxplanar/srxplanar.py

                # rv *= self.correction
        else:
            rv = image
            if flip is True:


I think I know what happened here. There were in the old days a program called fit2D and now a program called pyfai and they used different definitions for the origin (the zero) of the image from top left to bottom left or sthg like that, so there was a particular flipping that we would often find. In general, we would like the code to be more general, so a more complete refactor could be done, but I don't think it is worth it as not many people are using this atm. So if we can keep what you have done, because it is good and we have more tests, but make sure that the code kind of behaves the same way as before, so flip can be either True or False and when it is True it is the flip that Xiahao was doing, then if you can assure me that this is the case, I will just merge this.

stevenhua0320 · 2025-11-03T17:51:05Z

please see comment. I think we are very close to being able to merge this.

I have checked that the way the current code does is compatible to manual action done in imageJ does and the matrix values we tested should be right. Therefore, we can merge this.

stevenhua0320 added 3 commits October 24, 2025 22:35

fix: fix loadimage function to robustly load image.

c124f16

chore: add news item

757b223

chore: change docstring.

3f4f892

sbillinge reviewed Oct 25, 2025

View reviewed changes

tests: add test to loadimage

9a28a4b

sbillinge reviewed Oct 25, 2025

View reviewed changes

sbillinge reviewed Oct 26, 2025

View reviewed changes

test: change test format.

b2cc5c5

sbillinge reviewed Oct 26, 2025

View reviewed changes

stevenhua0320 added 2 commits October 25, 2025 23:48

test: change test to only catch function behavior.

0383f1f

fix: Fix directory display so that CI can identify

6124e29

sbillinge reviewed Oct 26, 2025

View reviewed changes

tests: change file-not-found to raise FileNotFound error.

709b48c

sbillinge reviewed Oct 27, 2025

View reviewed changes

stevenhua0320 added 3 commits October 27, 2025 17:40

tests: change copy of example to temp folder & not using logic in test

e34638e

chore: change test name

22a2bcd

chore: delete example.tiff

e5685ca

sbillinge reviewed Oct 29, 2025

View reviewed changes

tests/test_load_image.py Outdated Show resolved Hide resolved

tests/test_load_image.py Outdated Show resolved Hide resolved

tests/test_load_image.py Outdated Show resolved Hide resolved

tests/test_load_image.py Show resolved Hide resolved

tests: Change case 4 to FileNotFoundError.

8b00059

sbillinge reviewed Oct 30, 2025

View reviewed changes

Fix: Fix news message and rename to comply PEP-8 standard.

98c7220

sbillinge reviewed Oct 31, 2025

View reviewed changes

tests/test_load_image.py Outdated Show resolved Hide resolved

fix: change test logic to make it constant.

46f175b

sbillinge reviewed Oct 31, 2025

View reviewed changes

tests/test_load_image.py Outdated Show resolved Hide resolved

tests/test_load_image.py Outdated Show resolved Hide resolved

stevenhua0320 and others added 2 commits October 31, 2025 14:41

style: change parametrization style.

45f2952

Refactor image loading test with new expected values

4369cc2

Updated expected values for image loading tests and corrected the source file path.

sbillinge reviewed Oct 31, 2025

View reviewed changes

test: add starting point and last point for test.

556e141

sbillinge reviewed Nov 1, 2025

View reviewed changes

tests/test_load_image.py Show resolved Hide resolved

tests/test_load_image.py Outdated Show resolved Hide resolved

tests/test_load_image.py Outdated Show resolved Hide resolved

test: add test for flip_image mechanism.

ac7ece7

sbillinge reviewed Nov 1, 2025

View reviewed changes

test: add more points in validating flip.

1567b45

sbillinge reviewed Nov 1, 2025

View reviewed changes

style: change flip_image parameter to make non-flipping default.

03ae177

sbillinge reviewed Nov 3, 2025

View reviewed changes

style: change flip_image to pass attribute to make change

c8b83e5

sbillinge reviewed Nov 3, 2025

View reviewed changes

sbillinge merged commit 7b9a5ea into diffpy:main Nov 3, 2025
4 checks passed

		return LoadImage(cfg)


		# ----------------------------------------------------------------------

		# Unified test
		# ----------------------------------------------------------------------

		return image_loader.loadImage(tif_path)


		def build_loadimage_path(case_tag, home_dir):



		load_image_param = load_image_params = [
		("abs", PROJECT_ROOT / "docs/examples/KFe2As2-00838.tif"),

Fix: fix loadimage function to robustly load image #34

Fix: fix loadimage function to robustly load image #34

Uh oh!

Conversation

stevenhua0320 commented Oct 25, 2025

Uh oh!

codecov bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

stevenhua0320 commented Oct 25, 2025

Uh oh!

sbillinge commented Oct 25, 2025

Uh oh!

stevenhua0320 commented Oct 25, 2025

Uh oh!

sbillinge commented Oct 25, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

stevenhua0320 commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenhua0320 commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

stevenhua0320 commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge commented Oct 26, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 25, 2025 •

edited

Loading

stevenhua0320 commented Oct 26, 2025 •

edited

Loading

stevenhua0320 commented Oct 26, 2025 •

edited

Loading

stevenhua0320 commented Oct 26, 2025 •

edited

Loading